fix(exec): timeout Windows taskkill on the timeout path - #195
Merged
Conversation
Bound taskkillTree so a hung taskkill cannot block runCommandArgs after timeoutMs already fired. Default 5s, override with CLAWPATCH_TASKKILL_TIMEOUT_MS. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 availability
Contributor
|
Codex review: needs maintainer review before merge. Reviewed August 31, 2026, 8:17 PM ET / September 1, 2026, 00:17 UTC. ClawSweeper reviewWhat this changesThis PR bounds a hung Windows process-tree cleanup, terminates the direct child after failed cleanup, preserves Windows shell quoting, and adds tests, documentation, changelog text, and Windows CI coverage. Regression provenancePossible regression — probable (reproduction; reviewed change). No predecessor PR is attributed. Merge readiness✅ Ready for maintainer review Keep this PR open for ordinary maintainer review. The bounded Windows cleanup path is implemented, documented, covered by a Windows-specific test and CI job, and supported by a native Windows before/after CLI trace; no actionable patch defect was identified. Priority: P1 Review scores
Verification
How this fits togetherClawpatch’s shared command runner launches provider, Git, shell, and validation commands and returns a normalized command result to CLI workflows. On Windows, timeout handling invokes process-tree cleanup before returning control to the caller. flowchart LR
A[CLI workflow] --> B[Shared command runner]
B --> C{Command deadline reached?}
C -->|No| G[Command result]
C -->|Windows timeout| D[Process-tree cleanup]
D --> E[Cleanup deadline]
E --> F[Terminate direct child]
F --> G
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Land the focused timeout and quoting repair after ordinary exact-head maintainer review. Do we have a high-confidence way to reproduce the issue? Yes. The supplied native Windows before/after built-CLI trace gives a concrete reproduction path, and the prior source shows taskkill could wait without a cleanup deadline; this read-only review did not rerun it. Is this the best way to solve the issue? Yes. Bounding the cleanup helper while retaining best-effort tree termination and a direct-child fallback is a narrow repair to the shared timeout contract. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 09900a5edd2b. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (9 earlier review cycles; latest 8 shown)
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
Verify hanging cleanup invocation, reject unsupported timer delays, and cover the Windows caller in CI. Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
clawsweeper
Bot
added
the
merge-risk: 🚨 automation
clawsweeper
Bot
added
proof: sufficient
Contributor
|
Maintainer follow-up: completed the cleanup fix on this branch, preserving @SebTardif's contribution. The final head is Native Windows proof now exercises the built The new Windows CI job also exposed and now covers quoted shell-command handling. Final CI passes both Linux and Windows. Typecheck, lint, formatting, full tests, build, and packaged CLI smoke passed; native Windows executor coverage is 20 passed / 1 Unix-only skip. Codex autoreview is scoped-clean at the default P0 threshold. LAND recommended. This remains open and unmerged for the orchestrator. Land the shared executor change before the Go-mapper deadline in #194. |
steipete
added a commit
to SebTardif/clawpatch
that referenced
this pull request
Sep 1, 2026
Include the shared Windows executor fix from openclaw#195 so Go mapping has bounded cleanup. Verify hanging Go invocation and discard incomplete package output before falling back to files. Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
steipete
pushed a commit
that referenced
this pull request
Sep 1, 2026
Give Go package discovery a configurable two-minute deadline, reject unsupported timer values, and discard incomplete output before falling back to file-based package mapping. Include regression coverage, configuration documentation, and the changelog entry. Land after #195 so the shared Windows timeout cleanup is bounded. Verified approved head 9ff665e remains conflict-free with green Linux and Windows CI, existing macOS/native Windows built-CLI timeout proof, and completed Codex autoreview. Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Windows command deadlines could still leave Clawpatch hung while waiting for
taskkill /T /F. Cleanup now has its own five-second deadline (CLAWPATCH_TASKKILL_TIMEOUT_MS), kills a wedged cleanup process, and terminates the direct child even when tree cleanup fails. Invalid or unsupported timer values fall back to the default; descendant cleanup remains best effort iftaskkillis unavailable or hung.The regression coverage verifies that the hanging cleanup process actually starts, exercises the Windows
runCommandArgscaller, and checks that the direct child dies. A dedicated Windows executor CI job also exposed a pre-existing quoted-shell-command bug; the command runner now preservescmd.exequoting, with portable shell quoting in the test fixtures. Configuration docs and the changelog describe the behavior.Native Windows proof used the built CLI, a temporary Git repository with a local bare remote, and native
gh.exe/taskkill.exefault-injection shims that record their invocations before hanging. No real GitHub PR was created by the proof.Observed against the submitted implementation: the CLI printed
error: gh pr create failed: command timed out after 1000ms, but remained alive at the 10-second outer deadline. Against the corrected implementation: the same CLI exited with its expected GitHub-failure code7in 3130ms; the direct child was gone; theghshim ran once andtaskkillran twice with the expected/pid ... /T /Farguments. Final harness marker:CLI_EXITED_AND_CHILD_TERMINATED=true.Validation: native Windows executor tests passed (20 passed, 1 Unix-only skip), local macOS executor tests passed (19 passed, 2 Windows-only skips), and the Linux full suite passed (916 passed, 2 skipped), along with typecheck, lint, formatting, build, and packaged CLI smoke (13 features, including 3 CUDA). The final source also runs through GitHub's Linux and Windows CI jobs. Codex autoreview returned scoped-clean at its default P0 threshold.
Related Go-mapper timeout work: #194. Land this shared cleanup fix first. Original contribution by @SebTardif is retained in the commit history and changelog.